-
Notifications
You must be signed in to change notification settings - Fork 557
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add PillBadgeView to Python Panels #4909
Conversation
WalkthroughThe changes introduce a new React component named Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (4)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
11-22
: LGTM: Implementation is solid, with a minor suggestion for improvement.The component implementation is well-structured and follows React best practices. The use of
getComponentProps
for flexible styling is a good approach.One minor suggestion for improvement:
Consider adding default values or null checks for the properties destructured from
view
to handle cases where these properties might be undefined.Here's a suggested minor refactor:
const { text = '', color, variant, showIcon } = view; return ( <Box {...getComponentProps(props, "container")}> <PillBadge text={text} color={color} variant={variant} showIcon={showIcon} {...getComponentProps(props, "pillBadge")} /> </Box> );This ensures that
text
will always have a value, even if it's not provided in theview
object. You might want to add similar defaults forcolor
,variant
, andshowIcon
based on your specific requirements.e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1)
36-36
: LGTM: Property name updated correctly.The change from
persist
topersistent
appears to be a correct update to the dataset API usage. This ensures that the datasets created for testing are properly saved.Consider adding a brief comment explaining why persistence is necessary for these test datasets. This could improve code readability and maintainability.
+ # Ensure the dataset is saved persistently for the duration of the tests dataset.persistent = True
app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
52-52
: LGTM! The new PillBadgeView component is correctly exported.The export statement for
PillBadgeView
is correctly added and follows the same pattern as other exports in this file. This change aligns with the PR objectives to introduce the newPillBadgeView
component.Consider maintaining alphabetical order for exports to improve readability and ease of maintenance. The
PillBadgeView
export could be moved up a few lines to keep the list alphabetized.fiftyone/operators/types.py (1)
1478-1491
: Implementation looks good, with a minor suggestion for improvement.The
PillBadgeView
class is well-structured and inherits appropriately fromReadOnlyView
. The docstring provides clear information about the class's purpose and parameters. However, to enhance clarity and maintainability, consider explicitly defining the class-specific attributes (text
,color
,variant
,show_icon
) in the__init__
method, even if they're handled by the parent class.Example:
def __init__(self, text="Reviewed", color="primary", variant="outlined", show_icon=False, **kwargs): super().__init__(**kwargs) self.text = text self.color = color self.variant = variant self.show_icon = show_iconThis approach would make the class's specific properties more explicit and easier to understand for other developers.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (9)
- app/packages/components/src/components/PillBadge/PillBadge.tsx (1 hunks)
- app/packages/components/src/components/PillBadge/index.ts (1 hunks)
- app/packages/core/src/components/Modal/use-looker.ts (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1 hunks)
- app/packages/core/src/plugins/SchemaIO/components/index.ts (1 hunks)
- e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1 hunks)
- fiftyone/operators/types.py (1 hunks)
- fiftyone/utils/sam.py (1 hunks)
- fiftyone/utils/sam2.py (2 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/packages/components/src/components/PillBadge/index.ts
🧰 Additional context used
📓 Path-based instructions (5)
app/packages/components/src/components/PillBadge/PillBadge.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/components/Modal/use-looker.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.app/packages/core/src/plugins/SchemaIO/components/index.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1)
Pattern
**/*.{ts,tsx}
: Review the Typescript and React code for conformity with best practices in React, Recoil, Graphql, and Typescript. Highlight any deviations.
🔇 Additional comments (6)
app/packages/core/src/plugins/SchemaIO/components/PillBadgeView.tsx (1)
1-4
: LGTM: Import statements are appropriate and concise.The import statements are well-organized and include all necessary dependencies for the component's functionality.
e2e-pw/src/oss/specs/sidebar/frame-filtering.spec.ts (1)
Line range hint
1-78
: Overall file structure and content look good.The test file is well-structured and follows good testing practices:
- It uses Playwright's test framework with custom extensions.
- Creates two datasets with different frame filtering configurations for comprehensive testing.
- Includes separate tests for asserting enabled and disabled frame filtering behaviors.
- Follows TypeScript best practices with appropriate typing.
app/packages/core/src/components/Modal/use-looker.ts (1)
59-59
: LGTM. Verify consistency across the codebase.The change from
looker.updateSample(sample)
tolooker.updateSample(sample.sample)
looks correct and intentional. It suggests that theModalSample
type now has a nestedsample
property containing the actual sample data.To ensure consistency, let's verify the usage of
ModalSample
across the codebase:This will help us confirm that the change is consistent with other parts of the codebase and identify any potential areas that might need similar updates.
✅ Verification successful
Verified: Change is consistent across the codebase.
The update from
looker.updateSample(sample)
tolooker.updateSample(sample.sample)
aligns with theModalSample
type structure and is consistently applied throughout the codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the usage of ModalSample and updateSample method across the codebase # Search for ModalSample type usage echo "ModalSample type usage:" rg --type typescript "ModalSample" # Search for updateSample method calls echo "\nupdateSample method calls:" rg --type typescript "updateSample"Length of output: 264
Script:
#!/bin/bash # Description: Check the usage of ModalSample and updateSample method across the codebase # Search for ModalSample type usage echo "ModalSample type usage:" rg --type ts "ModalSample" # Search for updateSample method calls echo "\nupdateSample method calls:" rg --type ts "updateSample"Length of output: 9487
fiftyone/utils/sam.py (1)
274-286
: Excellent addition of prediction scores!The inclusion of
scores
in the output dictionary enhances the model's output by providing confidence metrics for each prediction. This addition aligns well with best practices in machine learning, offering users valuable information about the reliability of each prediction.fiftyone/utils/sam2.py (1)
Line range hint
204-219
: LGTM: Enhanced output with prediction scoresThe changes correctly implement the addition of scores to the output of the
_forward_pass_boxes
method. This enhancement provides valuable confidence information for the predictions.To ensure these changes are fully integrated, please run the following script to check if the
scores
are being used elsewhere in the codebase:If the script doesn't return any results, consider updating other parts of the codebase to utilize these scores for improved functionality.
✅ Verification successful
Action Required: Re-verify Usage of 'scores' in SAM2 Predictions
The previous attempt to search for
scores
usage failed due to an unrecognized file type. Please run the updated script below to accurately locate any references toscores
related to SAM2 predictions:
Verification Completed: 'scores' Usage Confirmed
The
scores
variable is correctly utilized withinfiftyone/utils/sam2.py
at lines 214-222. There are no additional references toscores
elsewhere in the codebase, ensuring that the integration is appropriate and isolated.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of 'scores' in relation to SAM2 predictions # Search for potential uses of 'scores' in Python files rg --type python -g '!test_*' -g '!*_test.py' -i '(sam2.*scores|scores.*sam2)' # Search for any modifications to the output structure of SAM2 predictions rg --type python -g '!test_*' -g '!*_test.py' -i '(sam2.*output|output.*sam2)'Length of output: 223
Script:
# #!/bin/bash # Description: Check for usage of 'scores' in relation to SAM2 predictions # Find all Python files excluding tests and search for 'scores' usage fd --extension py --exclude 'test_*' --exclude '*_test.py' | xargs rg -i '(sam2.*scores|scores.*sam2)' # Alternatively, using rg to search within .py files directly rg -t py -g '!test_*' -g '!*_test.py' -i '(sam2.*scores|scores.*sam2)'Length of output: 501
app/packages/components/src/components/PillBadge/PillBadge.tsx (1)
51-71
: Component correctly handles stringtext
prop.The rendering logic for when
text
is a string is appropriately implemented and follows best practices.
export default function PillBadgeView(props) { | ||
const { schema } = props; | ||
const { view = {} } = schema; | ||
const { text, color, variant, showIcon } = view; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider adding TypeScript types and using a named export.
While the component structure is good, it could benefit from the following improvements:
- Add TypeScript type annotations for props and destructured values to enhance type safety and developer experience.
- Consider using a named export instead of a default export for better refactoring capabilities and explicit imports.
Here's a suggested refactor:
import { Box } from "@mui/material";
import React from "react";
import { getComponentProps } from "../utils";
import PillBadge from "@fiftyone/components/src/components/PillBadge/PillBadge";
interface PillBadgeViewProps {
schema: {
view?: {
text?: string;
color?: string;
variant?: string;
showIcon?: boolean;
};
};
}
export function PillBadgeView({ schema }: PillBadgeViewProps) {
const { view = {} } = schema;
const { text, color, variant, showIcon } = view;
// ... rest of the component
}
const PillBadge = ({ | ||
text, | ||
color = "default", | ||
variant = "filled", | ||
showIcon = true, | ||
}: { | ||
text: string | string[] | [string, string][]; | ||
color?: string; | ||
variant?: "filled" | "outlined"; | ||
showIcon?: boolean; | ||
}) => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Consider simplifying the text
prop to improve maintainability.
The text
prop accepts multiple types (string
, string[]
, and [string, string][]
), which increases complexity in the component logic and can lead to potential errors. Simplifying the prop to accept a single data type or splitting the component into separate components for each case could enhance readability and maintainability.
onChange={(event) => { | ||
const selectedText = text.find( | ||
(t) => t[0] === event.target.value | ||
); | ||
setChipSelection(event.target.value); | ||
setChipColor(selectedText?.[1] ?? "default"); | ||
}} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add checks for selectedText
to prevent undefined access.
In the onChange
handler, selectedText
may be undefined
if text.find
does not find a matching element. Accessing selectedText[1]
without checking if selectedText
exists could lead to runtime errors. Consider adding a check to ensure selectedText
is defined before using it.
Here's an example of how to adjust the code:
onChange={(event) => {
const selectedText = text.find(
(t) => t[0] === event.target.value
);
setChipSelection(event.target.value);
- setChipColor(selectedText?.[1] ?? "default");
+ if (selectedText) {
+ setChipColor(selectedText[1]);
+ } else {
+ setChipColor("default");
+ }
}}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
onChange={(event) => { | |
const selectedText = text.find( | |
(t) => t[0] === event.target.value | |
); | |
setChipSelection(event.target.value); | |
setChipColor(selectedText?.[1] ?? "default"); | |
}} | |
onChange={(event) => { | |
const selectedText = text.find( | |
(t) => t[0] === event.target.value | |
); | |
setChipSelection(event.target.value); | |
if (selectedText) { | |
setChipColor(selectedText[1]); | |
} else { | |
setChipColor("default"); | |
} | |
}} |
const COLORS: { [key: string]: string } = { | ||
default: "#999999", | ||
primary: "#FFB682", | ||
error: "error", | ||
warning: "warning", | ||
info: "info", | ||
success: "#8BC18D", | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ensure COLORS
object contains valid CSS color values.
In the COLORS
object, the values for 'error'
, 'warning'
, and 'info'
are strings that may not correspond to valid CSS color values. Assigning these strings to the color
style property may not produce the desired effect. Consider replacing them with appropriate color codes or ensuring that the component interprets these string values correctly.
For example, you can update the COLORS
object with valid color codes:
const COLORS: { [key: string]: string } = {
default: "#999999",
primary: "#FFB682",
- error: "error",
- warning: "warning",
- info: "info",
+ error: "#f44336",
+ warning: "#ff9800",
+ info: "#2196f3",
success: "#8BC18D",
};
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
const COLORS: { [key: string]: string } = { | |
default: "#999999", | |
primary: "#FFB682", | |
error: "error", | |
warning: "warning", | |
info: "info", | |
success: "#8BC18D", | |
}; | |
const COLORS: { [key: string]: string } = { | |
default: "#999999", | |
primary: "#FFB682", | |
error: "#f44336", | |
warning: "#ff9800", | |
info: "#2196f3", | |
success: "#8BC18D", | |
}; |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Adds a
PillBadgeView
to Python Panels which is a MuiChip component capable of rendering itself as a dropdown menu of options, color-coded or not (if desired). The indicator icon is also optional and can be enabled or removed with ashowIcon
flagScreen.Recording.2024-10-10.at.12.54.39.AM.mov
Screen.Recording.2024-10-10.at.12.37.11.AM.mov
Summary by CodeRabbit
New Features
PillBadge
component for displaying customizable pill-shaped badges.PillBadgeView
component to renderPillBadge
within a structured layout.Bug Fixes
Documentation